Skip to content

Issue-60730: Addressing the situation where we were saying that (any … #61191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

dfperry5
Copy link
Contributor

@dfperry5 dfperry5 commented Sep 19, 2022

…P)? does not conform to P, instead we should offer the Force Optional error messaging

Addressing Issue-60730 (#60730). In this scenario, we were showing the error message error: Argument type '(any P)?' does not conform to expected type 'P' to the user. In the case outlined in the issue, a more informative message would be displayed if we applied the ForceOptional fix, which is exactly what this PR does.

Resolves: #60730

Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfperry5 Thanks for the contribution!
A couple of tips:

  • Take a look at the How do I format my changes section to make sure the new introduced changes nicely formatted.
  • We also need to add test cases: See Running test section. That will help you to run the tests locally using lit and make sure that things are working as expected.

If you have any question, don't hesitate to ask.
Hope that helps :)

…P)? does not conform to P, instead we should offer the Force Optional error messaging
@dfperry5 dfperry5 force-pushed the Issue-60730-ForceOptional-InsteadOf-Nonconformance branch 2 times, most recently from 6837ff1 to a2d1ef6 Compare September 22, 2022 02:12
@xedin xedin self-requested a review September 24, 2022 03:51
@xedin xedin self-requested a review October 4, 2022 05:36
@xedin
Copy link
Contributor

xedin commented Oct 4, 2022

@swift-ci please test

@xedin
Copy link
Contributor

xedin commented Oct 5, 2022

@swift-ci please test

}
func passLayersOfOptional(value: (any P)??) {
takesP(value)
// expected-error@-1 {{value of optional type '(any P)??' must be unwrapped to a value of type '(any P)?}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this is not great... Could you please leave a FIXME(diagnostics): here to to record multiple ForceUnwrap fixes based on number of optionals we need to unwrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do I add the "FIXME"? As a comment above the takesP() call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's good enough.

@LucianoPAlmeida
Copy link
Contributor

@swift-ci Please smoke test

@dfperry5
Copy link
Contributor Author

dfperry5 commented Oct 8, 2022

@LucianoPAlmeida / @xedin - not sure if this is a dumb question but when I look at the failure above for MacOS --> (https://ci.swift.org/job/swift-PR-macos-smoke-test/3584/consoleFull#-1537259845d6fdb6cb-f376-4f2e-8bce-d31c7304698b) -- the only error I'm seeing is:

UNSUPPORTED: lldb-api :: lang/swift/backwards_compatibility/simple/TestSwiftBackwardsCompatibilitySimple.py (368 of 369)
TIMEOUT: lldb-api :: lang/swift/late_dylib/TestSwiftLateDylib.py (369 of 369)
******************** TEST 'lldb-api :: lang/swift/late_dylib/TestSwiftLateDylib.py' FAILED ********************

and

Exit Code: -9
Timeout: Reached timeout of 600 seconds

Any idea on how I can fix that? Looked at it a little today and feeling stumped!

@xedin
Copy link
Contributor

xedin commented Oct 8, 2022

It’s just a flaky test.

@xedin
Copy link
Contributor

xedin commented Oct 8, 2022

@swift-ci please test macOS platform

@LucianoPAlmeida
Copy link
Contributor

@swift-ci Please test macOS platform

1 similar comment
@LucianoPAlmeida
Copy link
Contributor

@swift-ci Please test macOS platform

@LucianoPAlmeida
Copy link
Contributor

Looks like I'm not able to kick it again ... not sure why 😆

@LucianoPAlmeida
Copy link
Contributor

@swift-ci Please test macOS platform

@xedin
Copy link
Contributor

xedin commented Oct 8, 2022

Let’s give it time :)

@LucianoPAlmeida LucianoPAlmeida merged commit a29686f into swiftlang:main Oct 10, 2022
@dfperry5 dfperry5 deleted the Issue-60730-ForceOptional-InsteadOf-Nonconformance branch June 29, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error message for type mismatch between any P and (any P)?
3 participants